Skip to content

Fix fill values in all Omega fields#428

Open
xylar wants to merge 25 commits into
E3SM-Project:developfrom
xylar:omega/fix-fill-values
Open

Fix fill values in all Omega fields#428
xylar wants to merge 25 commits into
E3SM-Project:developfrom
xylar:omega/fix-fill-values

Conversation

@xylar

@xylar xylar commented Jun 3, 2026

Copy link
Copy Markdown

Fix fill-value handling across Omega so inactive parts of arrays hold well-defined, NetCDF-standard fill values instead of uninitialized memory or inconsistent local sentinels, initialize the Kokkos arrays backing fields at attach time, and mask cell, edge, and vertex state fields to their active layers after initialization. Based on the companion PR that fixes biharmonic velocity mixing at boundary vertices (peeled out of this branch in #456).

Motivation

Omega fields declared their fill values locally in each module with inconsistent values (-9.99e30, -9.99E+30, -999, etc.) that did not match the NetCDF-C standard. More critically, the Kokkos arrays backing those fields were never explicitly initialized: inactive ocean layers (outside a column's active range) held uninitialized memory rather than a well-defined sentinel, so output files contained garbage rather than a recognizable fill value. Switching from zero-initialization to fill-value initialization also exposed a latent class of bugs where edge and boundary-layer fields assumed zero at mesh boundaries, producing corrupted tendencies when those layers were no longer zero by default. This change standardizes the fill-value constants, auto-fills arrays at attach time, and masks cell, edge, and vertex state fields to their valid layer ranges (with the correct per-geometry boundary treatment) after IC/restart read.

Summary

Fix fill-value handling across Omega:

  • Add components/omega/src/base/FillValues.h with centralized constexpr fill value constants (FillValueI4, FillValueI8, FillValueR4, FillValueR8, FillValueReal) that exactly match the NetCDF-C NC_FILL_* / SCORPIO PIO_FILL_* values. A type-indexed variable template FillValue<T> provides the primary interface.
  • Remove the FillValue argument from Field::create() (and from Tracers::define() / TracerDefs.inc). The fill value is now deduced automatically from the array's element type at attachData() time.
  • Add auto-fill to Field::attachData<T>(): a private fillWithValue<T>() helper calls Kokkos::deep_copy(InDataArray, FillValue<ValType>) and records the value in FieldMeta["_FillValue"] (the CF/NetCDF standard attribute). An optional FillOnAttach = false argument covers the cases where attachData() is called only to re-point a field at an already-computed array (time-level updates in OceanState and Tracers, and the IO time-coordinate attach/assign ordering).
  • Add layer-masking helpers to VertCoord: zeroEdgeField and applyEdgeLayerMask for edge fields (three-zone), and applyCellLayerMask and applyVertexLayerMask for cell and vertex fields (two-zone). Cell and vertex fields have no zeroed boundary zone: a cell layer is simply active or inactive, and a boundary vertex with one or more active surrounding cells holds valid, generally non-zero data.
  • Add OceanState::applyLayerMasks(TimeLevel), a single entry point that applies applyEdgeLayerMask to NormalVelocity and applyCellLayerMask to PseudoThickness, Temperature, and Salinity; it is called from OceanInit after the IC/restart read. NormalVelocity is additionally masked in the OceanState constructor and after each updateVelocityByTend step in the time stepper to keep the three-zone invariant throughout the run.
  • Call zeroEdgeField on NormalVelocityTend and VelocityDel2Aux.Del2Edge at the start of each tendency computation, so boundary layers show 0 rather than FillValueReal as required for flux-type edge fields.
  • Read MinLayerCell, MaxLayerCell, BottomGeomDepth, RefPseudoThickness, and VertCoordMovementWeights from the InitialVertCoord stream, with robust fill-value-count detection so a missing or unreadable variable falls back to a sensible default (e.g. MinLayerCell at the surface, MaxLayerCell at NVertLayers) instead of propagating fill values into the vertical-coordinate setup.
  • Fix VertAdv::computeVerticalPseudoVelocity (DivHU): clamp the inner edge loop to K <= MaxLayerEdgeTop(JEdge) to avoid reading fill values from edges shallower than the owning cell.
  • Add FillValueTest CTest (8 MPI tasks) with six subtests: fill-constant equality with NC_FILL_*, attachData auto-fill, inactive cell-layer fill after VertCoord initialization, the three-zone NormalVelocity invariant after IC read and applyEdgeLayerMask, the two-zone applyCellLayerMask pattern, and the two-zone applyVertexLayerMask pattern.
  • Add the design document components/omega/doc/design/FillValues.md and update the Field, Tracer, and VertCoord developer/user guides to describe fill values and layer masking.

Layer masking approach

Fields span layers [0, NVertLayers), but the valid extent of each cell, edge, or vertex is set by the active layers of the neighboring cells. The masking helpers enforce this per geometry.

Edge fields use four per-edge layer indices (stored in VertCoord) that define three zones:

Index Meaning
MinLayerEdgeTop shallowest active layer of the shallower neighbor
MinLayerEdgeBot shallowest active layer of the deeper neighbor
MaxLayerEdgeTop deepest active layer of the shallower neighbor
MaxLayerEdgeBot deepest active layer of the deeper neighbor

Zone 1 — fully inactive (K < MinLayerEdgeTop or K > MaxLayerEdgeBot): both neighboring cells are inactive at this layer. These layers hold FillValueReal and must never participate in any computation.

Zone 2 — boundary layer (MinLayerEdgeTop <= K < MinLayerEdgeBot or MaxLayerEdgeTop < K <= MaxLayerEdgeBot): exactly one neighbor is active. For flux-type edge fields (NormalVelocity, NormalVelocityTend, Del2Edge) the physically correct value is 0 because no flux crosses a solid boundary. For thickness-interpolated fields (MeanPseudoThickEdge, FluxPseudoThickEdge) the value is genuinely undefined and retains FillValueReal.

Zone 3 — active (MinLayerEdgeBot <= K <= MaxLayerEdgeTop): both neighbors are active; the field carries a normally computed value.

VertCoord::zeroEdgeField sets zones 1+2 to 0 (used at the start of each tendency step for flux fields that are recomputed each step, where the attachData fill cannot be relied on). VertCoord::applyEdgeLayerMask imposes the full three-zone pattern (zone 1 → fill, zone 2 → 0, zone 3 → unchanged) and is applied after IC/restart read and after each velocity tendency update.

Cell and vertex fields use a simpler two-zone pattern, because there is no flux-through-boundary condition to enforce. applyCellLayerMask sets layers outside [MinLayerCell, MaxLayerCell] to FillValueReal and leaves the active range unchanged. applyVertexLayerMask sets layers outside [MinLayerVertexTop, MaxLayerVertexBot] to FillValueReal and leaves the active range unchanged; unlike edges there is no zeroed boundary zone, since a boundary vertex adjacent to at least one active cell holds valid, generally non-zero data.

Checklist

  • Documentation:
  • Linting
  • Testing
    • Add a comment to the PR titled Testing with the following:
      • Which machines CTest unit tests
        have been run on and indicate that are all passing.
      • The Polaris omega_pr test suite
        has passed, using the Polaris e3sm_submodules/Omega baseline
      • Document machine(s), compiler(s), and the build path(s) used for -p for both the baseline (Polaris e3sm_submodules/Omega) and the PR build
      • Indicate "All tests passed" or document failing tests
      • Document testing used to verify the changes including any tests that are added/modified/impacted.
    • New tests:
      • CTest unit tests for new features have been added per the approved design.

Fixes #347
Fixes #409

@xylar xylar force-pushed the omega/fix-fill-values branch from 7fc03ea to 8fc6203 Compare June 3, 2026 10:26
@xylar xylar self-assigned this Jun 3, 2026
@xylar xylar added the clean up label Jun 3, 2026
@xylar xylar force-pushed the omega/fix-fill-values branch from 8fc3284 to 6625253 Compare June 3, 2026 11:33
@mwarusz

mwarusz commented Jun 3, 2026

Copy link
Copy Markdown
Member

It looks to me like it should be possible to automatically deduce the right fill value from the array type when calling attachData and not have to specify it when creating fields. What do you think @xylar ?

@xylar xylar force-pushed the omega/fix-fill-values branch from 6625253 to 8cc604f Compare June 3, 2026 15:52
@xylar

xylar commented Jun 3, 2026

Copy link
Copy Markdown
Author

@mwarusz, great idea! I'll see if I can incorporate that quickly into this PR.

@mwarusz

mwarusz commented Jun 3, 2026

Copy link
Copy Markdown
Member

@xylar Here are some suggestions on how to do it cleanly in C++ that you might find useful. You could define the fill values in FillValues.h as follows:

// Primary template intentionally undefined.
template <typename T>
constexpr T FillValue;

template <>
constexpr I4 FillValue<I4> = -2147483647;             ///< NC_FILL_INT 

template<>
constexpr I8 FillValue<I8> = -9223372036854775806LL;  ///< NC_FILL_INT64

...

Then if T is an array the following code gets the fill value:

constexpr auto FillVal = FillValue<typename T::non_const_value_type>;

The main benefit of doing it this way is that you don't have to write a long chain of if constexpr statements and you only need to modify one place when adding a new fill value.

@xylar xylar force-pushed the omega/fix-fill-values branch from 8cc604f to 147144f Compare June 3, 2026 16:40
@xylar

xylar commented Jun 3, 2026

Copy link
Copy Markdown
Author

@mwarusz, I like that. I'll implement it and run some tests, then I'll ask you to review once I've got it working.

@xylar xylar force-pushed the omega/fix-fill-values branch 2 times, most recently from 5fb3358 to 29d8f99 Compare June 3, 2026 17:59
@xylar xylar requested review from alicebarthel and mwarusz June 3, 2026 17:59
@xylar

xylar commented Jun 3, 2026

Copy link
Copy Markdown
Author

@mwarusz and @alicebarthel, I got optimistic about nearly being done and assigned you to review. But I still have some kinks to work out. I'll take this out of draft mode once it's working (hopefully tomorrow) and ping you again for a review.

@xylar xylar force-pushed the omega/fix-fill-values branch 3 times, most recently from fba980b to 079b462 Compare June 6, 2026 16:21
@xylar xylar force-pushed the omega/fix-fill-values branch from 079b462 to 0804a22 Compare June 8, 2026 09:04
@xylar xylar marked this pull request as ready for review June 8, 2026 09:21
@xylar

xylar commented Jun 8, 2026

Copy link
Copy Markdown
Author

Testing

I tested this after rebasing onto develop to bring in #403.

I am using the polaris branch E3SM-Project/polaris#603 for testing to make sure that the higher-order horizontal advection fix still works with fill values.

CTest unit tests:

  • Machine: chrysalis
  • Compiler: oneapi-ifx
  • Build type: Release
  • Result: All tests passed
  • Log: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260608/omega-pr-fix-fill-values/build/ctests.log

Polaris omega_pr suite

  • Baseline workdir: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260607/omega-pr-higher-order-overflow
  • Baseline build: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260607/overflow/build
  • PR build: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260608/omega-pr-fix-fill-values/build
  • PR workdir: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260608/omega-pr-fix-fill-values
  • Machine: chrysalis
  • Partition: compute
  • Compiler: oneapi-ifx
  • Build type: Release
  • Log: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260608/omega-pr-fix-fill-values/polaris_omega_pr.o1230206
  • Result: All tests passed

@xylar

xylar commented Jun 8, 2026

Copy link
Copy Markdown
Author

@mwarusz and @alicebarthel, I know you may both be busy with work related to the All-hands but I would appreciate a review on this.

I had started to implement a proper treatment of the surface pressure -- initially just reading it in from the initial state if it is present. But that work ran into the problem of needing to define a fill value for SurfacePressure. Thus, I decided to work on this first. I will base my SurfacePressure work on this branch but would appreciate getting it in before too long.

At the same time, I recognize that these changes touch a fair bit of code, so a good review may require some time.

I am happy to explain my thinking, particularly regarding the handling of fields on edges.

@xylar xylar added the bug Something isn't working label Jun 8, 2026
@xylar xylar requested a review from brian-oneill June 10, 2026 09:05
@xylar

xylar commented Jun 10, 2026

Copy link
Copy Markdown
Author

@brian-oneill, I also added you as a reviewer. The changes here are extensive enough that it feels like one more pair of eyes on the code would be a good idea.

xylar and others added 23 commits July 2, 2026 07:50
This merge adds 5 constexpr fill value constants (FillValueI4,
FillValueI8, FillValueR4, FillValueR8, FillValueReal) in namespace
OMEGA, wrapping PIO_FILL_* from SCORPIO.
This merge adds private fillWithValue<T>() template helper
(using if constexpr on T::value_type) and a call to it at the end
of attachData<T>(). Arrays are now auto-filled with the declared
fill value at attach time.
This merge removes all local fill value declarations and replaces
references with the centralized constants.
In some cases we definitely don't want to overwrite fields we attach
with fill values.

Fix 2 such cases in Tracers and OceanState
We don't want fill values at every boundary edge becausee these
are also valid.  Instead, we want zeros except for edges between
two inactive cells (e.g. both adjacent cells are below bathymetry)
Rather than assuming that the NormalVelocity read from an initial
condition has correct masking, we ensure that it has fill values
for inactive layers, zeros at boundary edges (both adjacent to
land and to bathymetry), and leave its values unchanged for active,
non-boundary layers.
We need to ensure that it is zero at active boundary edges.
Co-authored-by: Maciej Waruszewski <mwarusz@igf.fuw.edu.pl>
Add Test 5 and Test 6 to FillValueTest.cpp covering the cell and vertex
layer-mask methods added in 27c02f5. Each test fills a synthetic field
with a sentinel value, applies the mask, and verifies the resulting zones:
cells expect FillValueReal in inactive layers and the sentinel in active
layers; vertices expect the 3-zone pattern (fill / 0 / sentinel) matching
the existing edge-mask test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document applyCellLayerMask and applyVertexLayerMask alongside the edge
helpers in Section 4.5, and describe the OceanState::applyLayerMasks driver
called from ocnInit. Correct stale facts in Section 5: the test lives at
test/ocn/FillValueTest.cpp, uses direct comparison with Error accumulation,
and now includes cell and vertex mask cases (5.5, 5.6); remove the
unimplemented NetCDF attribute test. Update Requirement 2.7 for the expanded
cell/vertex coverage.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a Layer masking subsection to the VertCoord devGuide describing
zeroEdgeField, applyEdgeLayerMask, applyCellLayerMask, and applyVertexLayerMask,
their zones, the inclusive active-layer convention, and the applyLayerMasks
driver. Add a userGuide note explaining that inactive layers carry the fill
value and boundary layers carry 0 in output.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@xylar xylar force-pushed the omega/fix-fill-values branch from 58bd787 to b569502 Compare July 2, 2026 12:51
@xylar

xylar commented Jul 2, 2026

Copy link
Copy Markdown
Author

Latest Testing

I tested this including the fix in #456

CTest unit tests:

  • Machine: chrysalis
  • Compiler: oneapi-ifx
  • Build type: Release
  • Result: All tests passed
  • Log: /gpfs/fs1/home/ac.xylar/e3sm_work/polaris/main/build_omega/build_chrysalis_oneapi-ifx/ctests.log

Polaris omega_pr suite

  • Baseline workdir: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260630/omega-pr-develop
  • Baseline build: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260630/omega-pr-develop/build
  • PR build: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260702/omega-pr-fill-values2/build
  • PR workdir: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260702/omega-pr-fill-values2
  • Machine: chrysalis
  • Partition: compute
  • Compiler: oneapi-ifx
  • Build type: Release
  • Log: /lcrc/group/e3sm/ac.xylar/polaris_1.0/chrysalis/test_20260702/omega-pr-fill-values2/polaris_omega_pr.o1244640
  • Result: All tests passed

@xylar

xylar commented Jul 2, 2026

Copy link
Copy Markdown
Author

My hope is that this can be merged right after #456 since the two should ideally be tested together.

@xylar xylar marked this pull request as ready for review July 2, 2026 13:08
@xylar xylar requested a review from sbrus89 July 6, 2026 08:35
@xylar

xylar commented Jul 6, 2026

Copy link
Copy Markdown
Author

@sbrus89 and @alicebarthel, I believe this and #456 are waiting on your final approval before getting merged. Do you think it will be possible to get them approved and merged today? They're holding up a lot of other work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working clean up

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure that invalid cells and levels are populated with FillValue Invalid data below MaxLayerCell is not being masked as invalid in output

6 participants